Skip to content

BREAKING: Remove namespaced permissions#1337

Merged
rekmarks merged 8 commits into
mainfrom
remove-namespaced-permissions
May 17, 2023
Merged

BREAKING: Remove namespaced permissions#1337
rekmarks merged 8 commits into
mainfrom
remove-namespaced-permissions

Conversation

@rekmarks
Copy link
Copy Markdown
Member

@rekmarks rekmarks commented May 7, 2023

Description

Removes namespaced permissions from the PermissionController for the reasons described in #1323. As a consequence, also removes the notion of "target keys", which only existed to support namespaced methods. Controller state is not impacted by these changes.

Changes

  • BREAKING: Remove namespaced permissions
    • Namespaced permissions are no longer supported. Consumers should replace namespaced permissions with equivalent caveat-based implementations.
  • BREAKING: Remove targetKey concept
    • The target key/name distinction only existed to support namespaced permissions, which are removed as of this release. Henceforth, permissions only have "names".
    • The targetKey property of permission specifications has been renamed to targetName.

References

Fixes #1323

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@rekmarks rekmarks force-pushed the remove-namespaced-permissions branch from 9bf9690 to 1ad521c Compare May 7, 2023 05:22
@rekmarks rekmarks force-pushed the remove-namespaced-permissions branch from a5ef4f3 to 1faa530 Compare May 7, 2023 05:28
@rekmarks rekmarks marked this pull request as ready for review May 7, 2023 05:29
@rekmarks rekmarks requested a review from a team as a code owner May 7, 2023 05:29
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great - just a few small comments.

Comment thread packages/permission-controller/src/Permission.ts Outdated
Comment thread packages/permission-controller/src/PermissionController.ts
@legobeat

This comment was marked as resolved.

@FrederikBolding
Copy link
Copy Markdown
Member

For the sake of asking the question: As this is breaking API, would it make sense to push a release of the changes of @metamask/permssion-controller since last release?

https://github.com/MetaMask/core/commits/main/packages/permission-controller

I don't see a good reason tbh, there hasn't been any meaningful changes since last release.

rekmarks and others added 2 commits May 16, 2023 08:12
@rekmarks rekmarks requested a review from FrederikBolding May 16, 2023 15:21
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rekmarks rekmarks merged commit e91871a into main May 17, 2023
@rekmarks rekmarks deleted the remove-namespaced-permissions branch May 17, 2023 16:17
github-merge-queue Bot pushed a commit that referenced this pull request Mar 26, 2026
## Explanation

This fixes an issue in changelogs where changelog entries may be
incorrectly moved after merging into the main branch. Given the
following (partial) changelog for example:

```md
## [Unreleased]

### Changed

- Changelog entry A ([#1234](...))
- Changelog entry B ([#5678](...))
- Changelog entry C ([#1337](...))
```

After a release, this changelog may look like this:

```diff
  ## [Unreleased]

+ ## [1.0.0]

  ### Changed

  - Changelog entry A ([#1234](...))
  - Changelog entry B ([#5678](...))
  - Changelog entry C ([#1337](...))
```

But if another PR based on the original changelog also adds an entry to
the unreleased section like this:

```diff
  ## [Unreleased]

  ### Changed

  - Changelog entry A ([#1234](...))
  - Changelog entry B ([#5678](...))
  - Changelog entry C ([#1337](...))
+ - Changelog entry D ([#0000](...))
```

The final changelog will look like this, since Git can merge the entry
based on the other entries around it:

```md
## [Unreleased]

## [1.0.0]

### Changed

- Changelog entry A ([#1234](...))
- Changelog entry B ([#5678](...))
- Changelog entry C ([#1337](...))
- Changelog entry D ([#0000](...))
```

Here it appears that the release included "entry D", but in reality that
was meant to be under the unreleased section.

To address this issue, I've added a simple workflow that checks for
changelog diffs. It uses a minimal algorithm to parse the markdown
files, find entries that are included in unreleased originally, and
checks if they still are after merging. If not, the workflow will fail
and show an error such as this:

> Checking changelog file: packages/some-package/CHANGELOG.md
> The following lines added in the PR are missing from the "Unreleased"
section after merge:
>
>    - Changelog entry D ([#0000](...))
>
> Please update your pull request and ensure that new changelog entries
remain in the "Unreleased" section.

Long term we should solve this problem by using changesets instead, in
which case the unreleased changes will never be merged or conflict with
each other, but until we figure out how to implement those in our
release workflows, we can use this workflow.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs)
- [ ] I've introduced [breaking
changes](https://github.com/MetaMask/core/tree/main/docs/breaking-changes.md)
in this PR and have prepared draft pull requests for clients and
consumer packages to resolve them

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk CI-only change that adds a new workflow gate; main failure
mode is false positives/negatives that could block merges.
> 
> **Overview**
> Adds a new composite GitHub Action (`check-merge-queue-changelogs`)
and Node script to detect when changelog lines introduced in a PR get
moved out of the `## [Unreleased]` section after merge (a common
merge-queue conflict artifact).
> 
> Updates `lint-build-test.yml` to run a new `validate-changelog-diffs`
job on `pull_request` and `merge_group` events, failing the workflow
when modified `CHANGELOG.md` files lose newly-added Unreleased entries
in the merged result.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
ef5a189. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove namespaced permissions from the PermissionController

3 participants